Skip to content

done#6288

Open
nesterka375-bit wants to merge 7 commits into
mate-academy:masterfrom
nesterka375-bit:develop
Open

done#6288
nesterka375-bit wants to merge 7 commits into
mate-academy:masterfrom
nesterka375-bit:develop

Conversation

@nesterka375-bit

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because a couple of core requirements from the task description and checklist are not yet met. The most important blocker is [CHECKLIST ITEM #5]: all hover changes must be smooth over 300ms; currently .card and .card__button have hover effects (scale, box-shadow, background-color, text color) but no corresponding transition on their base state, so these interactions are abrupt instead of smoothly animated. To fix this, add something like transition: transform 0.3s ease, box-shadow 0.3s ease, color 0.3s ease; on .card in card.scss and transition: background-color 0.3s ease, color 0.3s ease, box-shadow 0.3s ease; on .card__button, and ensure these styles are correctly imported into main.scss so they apply on the page. A second blocker is [CHECKLIST ITEM #2]: repeated/special colors must use variables; you still hard-code #00acdc and #34568b in card.scss, header.scss, and reset.scss instead of using the existing $accent-color / $blue-accent, and adding a new variable (e.g. $card-title-hover) for #34568b in variables.scss. Please replace those literal colors with the appropriate SCSS variables everywhere they appear so the color palette is centralized and consistent. As a minor improvement, you may also want to review the Backstop visual regression report to ensure your layout (especially the <main> section at 1200px) matches the reference design, but the main blockers remain the hover transitions and variable usage. Overall, your structure, grid, data-qa attributes, and BEM usage look strong—once you smooth out the hover effects and refactor the magic colors into variables, this solution will fully meet the task requirements.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.html
Comment thread src/index.html
Comment thread src/styles/card/card.scss
Comment thread src/styles/card/card.scss
Comment thread src/styles/card/card.scss Outdated
Comment thread src/styles/reset/reset.scss
Comment thread src/styles/reset/reset.scss
Comment thread src/styles/reset/reset.scss
Comment thread src/styles/reset/reset.scss
Comment thread src/styles/variables/variables.scss

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one core functional requirement is still not implemented: the card title color does not change on hover as specified. The task explicitly requires .card:hover .card__title to change its text color to #34568b (your $card-title-hover), but your current hover rule targets .card__name, which does not exist in the markup. This violates the description and the hover behavior requirement in [CHECKLIST ITEM #5], so it is a critical blocker rather than a cosmetic issue.

To fix this, update the selector in src/styles/card/card.scss from .card__name to .card__title inside the &:hover block, so that the hover state actually applies to the title element you have in the HTML. Also make sure the corresponding rule in index.html (if any inline or class mismatch exists) consistently uses .card__title for the title element. Once this selector targets .card__title, the color will change on hover and the existing transition: color 300ms ease; will ensure the change is smooth. Everything else in your implementation looks aligned with the requirements, so after this adjustment you should be in good shape.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.html
Comment thread src/styles/card/card.scss
Comment thread src/styles/card/card.scss Outdated
Comment thread src/styles/card/card.scss Outdated
Comment thread src/styles/card/card.scss Outdated
Comment thread src/index.html Outdated
Comment thread src/styles/catalog/catalog.scss Outdated
Comment thread src/styles/catalog/catalog.scss Outdated
Comment thread src/styles/catalog/catalog.scss Outdated
Comment thread src/styles/catalog/catalog.scss Outdated
Comment thread src/styles/catalog/catalog.scss Outdated

@natalia-klonowska natalia-klonowska left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jeszcze zaktualizuj swoje linki do demo i raportu testów ponownie wykonując deploy komendą npm run deploy

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants